-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Enhancement] Implement pruning for neural sparse search #988
[Enhancement] Implement pruning for neural sparse search #988
Conversation
1e55b7c
to
46b9d9a
Compare
This PR is ready for review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide an overview of how the overall API will look? I initially thought this change would only affect the query side, but it seems it will also modify the parameters for neural_sparse_two_phase_processor
.
Additionally, the current implementation appears to be focused on two-phase processing with different strategies for splitting vectors, rather than a combination of pruning and two-phase processing?
src/main/java/org/opensearch/neuralsearch/processor/factory/SparseEncodingProcessorFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/util/prune/PruneUtils.java
Outdated
Show resolved
Hide resolved
Based on our benchmark results in #946 , when searching, applying prune to 2-phase search has superseded applying it to neural sparse query body, on both precision and latency. Therefore, enhancing the existing 2-phase search pipeline makes more sense.
The existing two-phase use max_ratio prune criteria. And now we add supports for other criteria as well |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #988 +/- ##
============================================
+ Coverage 80.47% 81.27% +0.79%
- Complexity 1000 1054 +54
============================================
Files 78 80 +2
Lines 3411 3535 +124
Branches 578 611 +33
============================================
+ Hits 2745 2873 +128
+ Misses 425 423 -2
+ Partials 241 239 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from minor comment, why this PR is trying to merge into main
?
If this changes API that used to define the processor, it should be checked with application security and for that we need to merge to feature branch in main repo, and only after that's cleared from feature branch to main.
src/main/java/org/opensearch/neuralsearch/processor/NeuralSparseTwoPhaseProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/SparseEncodingProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/factory/SparseEncodingProcessorFactory.java
Outdated
Show resolved
Hide resolved
); | ||
} else { | ||
// if we don't have prune type, then prune ratio field must not have value | ||
if (config.containsKey(PruneUtils.PRUNE_RATIO_FIELD)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can merge this if with a previous else
and have one single else if
block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else means PruneType is NONE right? It seems can be moved to https://github.com/opensearch-project/neural-search/pull/988/files#diff-8453ea75f8259ba96c246d483b2de9e21601fb9c3d033e8902756f5d101f2238R262 when validating the input ratio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can merge this if with a previous else and have one single else if block
ack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else means PruneType is NONE right? It seems can be moved to https://github.com/opensearch-project/neural-search/pull/988/files#diff-8453ea75f8259ba96c246d483b2de9e21601fb9c3d033e8902756f5d101f2238R262 when validating the input ratio.
We want to validate that the PRUNE_RATIO field is not provided. Any values will be illegal
src/main/java/org/opensearch/neuralsearch/util/prune/PruneType.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/util/prune/PruneUtils.java
Outdated
Show resolved
Hide resolved
|
||
switch (pruneType) { | ||
case TOP_K: | ||
return pruneRatio > 0 && pruneRatio == Math.floor(pruneRatio); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return pruneRatio > 0 && pruneRatio == Math.floor(pruneRatio); | |
return pruneRatio > 0 && pruneRatio == Math.rint(pruneRatio); |
this is more reliable for float numbers, otherwise there is a chance of false positive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem correct to replace the floor to rint, from the definition, rint will give a even number if there are two values same close to the input value, I tested with input 3.5, floor result is 3 but rint result is 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please give an example of false positive?
} | ||
} | ||
|
||
switch (pruneType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, can we use map instead of switch?
@martin-gaievski Thanks for the comments. We didn't create feature branch because there is no other contributors working on this and we regard the PR branch as feature branch. I'm on PTO this week, will follow the app sec issue and solve the comments next week. |
|
||
switch (pruneType) { | ||
case TOP_K: | ||
return pruneRatio > 0 && pruneRatio == Math.floor(pruneRatio); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem correct to replace the floor to rint, from the definition, rint will give a even number if there are two values same close to the input value, I tested with input 3.5, floor result is 3 but rint result is 4.
src/main/java/org/opensearch/neuralsearch/util/prune/PruneUtils.java
Outdated
Show resolved
Hide resolved
* @param pruneType The type of prune strategy | ||
* @throws IllegalArgumentException if prune type is null | ||
*/ | ||
public static String getValidPruneRatioDescription(PruneType pruneType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] this can be refactored to a static map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to the discussion with Martin at above
); | ||
} else { | ||
// if we don't have prune type, then prune ratio field must not have value | ||
if (config.containsKey(PruneUtils.PRUNE_RATIO_FIELD)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else means PruneType is NONE right? It seems can be moved to https://github.com/opensearch-project/neural-search/pull/988/files#diff-8453ea75f8259ba96c246d483b2de9e21601fb9c3d033e8902756f5d101f2238R262 when validating the input ratio.
src/main/java/org/opensearch/neuralsearch/util/prune/PruneUtils.java
Outdated
Show resolved
Hide resolved
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
2a3e2cf
to
0d928a9
Compare
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let minor comment. Relaxing the potential merge blocker, looks like from app sec point of view the risk of this enhancement is low
App security flagged this as low risk change, removing the blocker
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Signed-off-by: zhichao-aws <zhichaog@amazon.com>
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-988-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e8fe2847a5237a03edd414a333799f7a5d2d8c7d
# Push it to GitHub
git push --set-upstream origin backport/backport-988-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x Then, create a pull request where the |
* add impl Signed-off-by: zhichao-aws <zhichaog@amazon.com> * add UT Signed-off-by: zhichao-aws <zhichaog@amazon.com> * rename pruneType; UT Signed-off-by: zhichao-aws <zhichaog@amazon.com> * changelog Signed-off-by: zhichao-aws <zhichaog@amazon.com> * ut Signed-off-by: zhichao-aws <zhichaog@amazon.com> * add it Signed-off-by: zhichao-aws <zhichaog@amazon.com> * change on 2-phase Signed-off-by: zhichao-aws <zhichaog@amazon.com> * UT Signed-off-by: zhichao-aws <zhichaog@amazon.com> * it Signed-off-by: zhichao-aws <zhichaog@amazon.com> * rename Signed-off-by: zhichao-aws <zhichaog@amazon.com> * enhance: more detailed error message Signed-off-by: zhichao-aws <zhichaog@amazon.com> * refactor to prune and split Signed-off-by: zhichao-aws <zhichaog@amazon.com> * changelog Signed-off-by: zhichao-aws <zhichaog@amazon.com> * fix UT cov Signed-off-by: zhichao-aws <zhichaog@amazon.com> * address review comments Signed-off-by: zhichao-aws <zhichaog@amazon.com> * enlarge score diff range Signed-off-by: zhichao-aws <zhichaog@amazon.com> * address comments: check lowScores non null instead of flag Signed-off-by: zhichao-aws <zhichaog@amazon.com> --------- Signed-off-by: zhichao-aws <zhichaog@amazon.com> (cherry picked from commit e8fe284)
* [Enhancement] Implement pruning for neural sparse search (#988) * add impl Signed-off-by: zhichao-aws <zhichaog@amazon.com> * add UT Signed-off-by: zhichao-aws <zhichaog@amazon.com> * rename pruneType; UT Signed-off-by: zhichao-aws <zhichaog@amazon.com> * changelog Signed-off-by: zhichao-aws <zhichaog@amazon.com> * ut Signed-off-by: zhichao-aws <zhichaog@amazon.com> * add it Signed-off-by: zhichao-aws <zhichaog@amazon.com> * change on 2-phase Signed-off-by: zhichao-aws <zhichaog@amazon.com> * UT Signed-off-by: zhichao-aws <zhichaog@amazon.com> * it Signed-off-by: zhichao-aws <zhichaog@amazon.com> * rename Signed-off-by: zhichao-aws <zhichaog@amazon.com> * enhance: more detailed error message Signed-off-by: zhichao-aws <zhichaog@amazon.com> * refactor to prune and split Signed-off-by: zhichao-aws <zhichaog@amazon.com> * changelog Signed-off-by: zhichao-aws <zhichaog@amazon.com> * fix UT cov Signed-off-by: zhichao-aws <zhichaog@amazon.com> * address review comments Signed-off-by: zhichao-aws <zhichaog@amazon.com> * enlarge score diff range Signed-off-by: zhichao-aws <zhichaog@amazon.com> * address comments: check lowScores non null instead of flag Signed-off-by: zhichao-aws <zhichaog@amazon.com> --------- Signed-off-by: zhichao-aws <zhichaog@amazon.com> (cherry picked from commit e8fe284) * fix toList for jvm version Signed-off-by: zhichao-aws <zhichaog@amazon.com> * adapt for the gap of batch ingest between 2.x main Signed-off-by: zhichao-aws <zhichaog@amazon.com> --------- Signed-off-by: zhichao-aws <zhichaog@amazon.com>
Description
Implement prune for sparse vectors, to save disk space and accelerate search speed with small loss on search relevance. #946
Related Issues
#946
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.